Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive schemes from endpoint :url #222

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

slissner
Copy link

@slissner slissner commented Feb 9, 2019

Summary

When handling url schemes (http/https), phoenix swagger currently does only take into account if there is set up a :https server in the endpoint config. Instead, it should first lookup if under the endpoint :url config a custom scheme is defined.

Problem

Currently, phoenix swagger is generating the base url with scheme “https” only if in the phoenix endpoint_config it is set as distinct :https server. (see https://github.com/xerions/phoenix_swagger/blob/master/lib/mix/tasks/swagger.generate.ex#L178) The collect_host_from_endpoint. It does not consider the endpoint :url config, in particular it ignores :scheme (see below).

On the other side, it is a common scenario to run one's application behind a (https) reverse proxy. It is therefore in the general interest, to allow generating https base paths for the client communication – although the phoenix server is running only in https.

The phoenix docs are clear on the issue. It says that :url is for “generating URLs throughout the app”. https://hexdocs.pm/phoenix/Phoenix.Endpoint.html Moreover, the docs explicitly mention the reverse proxy issue:

The :scheme option accepts "http" and "https" values. Default value is infered from top level :http or :https option. It is useful when hosting Phoenix behind a load balancer or reverse proxy and terminating SSL there.

This PR alters the behavior of phoenix swagger to that extent, that first the configuration in :url is taken into account.

Copy link
Contributor

@mbuhot mbuhot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me 👍

@@ -3,3 +3,5 @@
/deps
erl_crash.dump
*.ez
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest ignoring IDE generated files globally in your git config so they don’t need to be ignored in each repo.

@@ -188,6 +189,13 @@ defmodule Mix.Tasks.Phx.Swagger.Generate do
swagger_map # host / port may be {:system, "ENV_VAR"} tuples or loaded in Endpoint.init callback
end

swagger_map =
if scheme == "https" do
Map.put_new(swagger_map, :schemes, ["https", "http"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is plain http included for local development, even if the production endpoint is always https ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants